Network Provider: Cloud subnets refactor #1545

gildub · 2017-06-15T07:04:56Z

Networks > Subnets > add/edit one

Replaces $scope with this
Adds ng-cloak and form-change
Removes obsolete miqrequire

Depends upon #1508

https://bugzilla.redhat.com/show_bug.cgi?id=1467725

gildub · 2017-06-17T09:59:58Z

@dclarizio, would you please assign it, thanks

miq-bot · 2017-06-20T06:26:21Z

@gildub unrecognized command 'label', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

miq-bot · 2017-06-20T06:28:06Z

@gildub Cannot apply the following label because they are not recognized: networkings

gildub · 2017-06-20T06:28:58Z

@miq-bot add_label networks

himdel · 2017-06-23T12:31:50Z

Test failure looks valid...

gildub · 2017-06-28T03:13:30Z

@himdel, all green now.

himdel · 2017-06-28T12:19:19Z

app/assets/javascripts/controllers/cloud_subnet/cloud_subnet_form_controller.js

+      API.get("/api/cloud_subnets/" + cloudSubnetFormId + "?expand=resources&attributes=ext_management_system.name,cloud_tenant.name,cloud_network.name").then(function(data) {
+        vm.cloudSubnetModel.name = data.name;
+        vm.cloudSubnetModel.ems_name = data.ext_management_system.name;
+        vm.cloudSubnetModel.tenant_name = angular.isDefined(data.cloud_tenant) ? data.cloud_tenant.name : undefined;


FYI we're trying to remove angular.isDefined, this could be just data.cloud_tenant ? data.cloud_tenant.name : undefined.

Oh, that was actually recommended recently, there are cases where the variable ends being undefined and would otherwise breaks.

Well, if you need to check for definedness, use data.cloud_tenant !== undefined.

But since you just return undefined when cloud tenant is undefined, you don't need to check that.

This could also be written as data.cloud_tenant && data.cloud_tenant.name.

himdel · 2017-06-28T12:31:38Z

Not sure the API is quite ready for this...

I have 4 Network managers, choosing 2 of them gives me 0 tentants (that's probably OK), another one is fine, has tenants, and another one gives me...

GET http://localhost:3000/api/providers/10000000000019/cloud_tenants?expand=resources&attributes=id,name

{"error":{"kind":"internal_server_error","message":"undefined method `all' for nil:NilClass","klass":"NoMethodError"}}

After that, chosing the one with tenants and chosing a tenant and filling out a few field by random, I hit Save. .. and all I get is an error screen with Invalid input [cloud_subnet/create]. No further info.

So.. I'm guessing there's some validation missing?

himdel · 2017-06-28T12:32:39Z

Oh.. saving is not done via the API, it's still using miqAjaxButton..

so .. the Invalid input bit dies with this...

F, [2017-06-28T12:30:08.856331 #16556] FATAL -- : Error caught: [RuntimeError] Invalid input
/home/himdel/manageiq-ui-classic/app/controllers/mixins/checked_id_mixin.rb:72:in `find_record_with_rbac'
/home/himdel/manageiq-ui-classic/app/controllers/cloud_subnet_controller.rb:229:in `new_form_params'
/home/himdel/manageiq-ui-classic/app/controllers/cloud_subnet_controller.rb:65:in `create'

The data that got sent on save was..

name:abc
ems_id:10000000000028
cloud_tenant_id:10r11
network_id:c0f0db9c-846f-4d4e-b058-0db5bfb2cb90
dhcp_enabled:true
network_protocol:ipv4
gateway:127.0.0.4
cidr:32

(I'm not sure these values are valid for those fields, but there's no validation, so I'll assume they are, or the server needs to return a better message :).)

himdel · 2017-06-28T12:35:53Z

app/controllers/cloud_subnet_controller.rb

@@ -261,7 +225,7 @@ def new_form_params
    if params[:gateway]
      options[:gateway] = params[:gateway].blank? ? nil : params[:gateway]
    end
-    options[:ip_version] = params[:network_protocol]
+    options[:ip_version] = params[:network_protocol] =~ /4/ ? 4 : 6
    options[:cloud_tenant] = find_record_with_rbac(CloudTenant, params[:cloud_tenant_id]) if params[:cloud_tenant_id]


.. probably caused by missing from_cid here.

Gotcha, thanks.

gildub · 2017-07-05T00:07:41Z

@himdel, this PR depends upon #1508 which is taking care of the API get parts. Saving cannot be done by API because changes of updating model through raw commands calls wrapped in task queues. Let's wait for PR#1508 to be merged.

miq-bot · 2017-07-05T15:09:22Z

This pull request is not mergeable. Please rebase and repush.

gildub · 2017-07-06T13:42:15Z

@himdel, could you please re-review? Thanks

himdel · 2017-07-11T10:03:32Z

@gildub Still missing that from_cid.

gildub · 2017-07-12T00:55:32Z

@himdel, I've rebased and re-tested again, the creation works fine, I'm not getting any issues or any missing from_cid. Could you please double check?

himdel · 2017-07-12T11:42:14Z

@gildub Set your database to REGION other than zero and you will, sorry :).

And yes, you need to use a REGION different than zero, otherwise you'll be missing from_cid from everywhere, since it is not needed for region 0.

Also, none of the other problems I mentioned are fixed, I'm still getting that undefined method 'all' for nil:NilClass when I select a different network manager, and there is no validation on the fields.

And the tests are failing.

Why am I even looking at this?

gildub · 2017-07-14T00:55:20Z

@himdel, the problem(s) your mentioned aren't fixed because I couldn't reproduce it (or them) and because of lack of context/description. Also the tests are failing because an unrelated issue, and this is actually happening way too often maybe because we have cross repos dependencies.

Now that said:

I'm going to check the support feature mixin because that's supposed to take care of non supported provider
What's the story with the database REGION? I never had to play with this, do you have any reference or documentation link?

Cheers,
Gilles

PS: BTW, this is a re-factor!

himdel · 2017-07-14T11:51:19Z

Also the tests are failing because an unrelated issue, and this is actually happening way too often maybe because we have cross repos dependencies.

Ah, thanks for restarting :). Yes, I feel your pain, it's too often :).

I'm going to check the support feature mixin because that's supposed to take care of non supported provider

Thanks.. if it helps, that problematic provider...

Both providers I can see there are ManageIQ::Providers::Openstack::NetworkManager. The failing one is called "OSP Director Network Manager" the working one "OpenStack Network Manager" but that probably doesn't help.

It looks like the important part is that for the failing one, manager.cloud_tenants is nil, whereas for the other one, it returns a collection of 5 items.

I've created an issue, looks like an API bug.. ManageIQ/manageiq#15569

What's the story with the database REGION? I never had to play with this, do you have any reference or documentation link?

The only documentation I could find is http://manageiq.org/docs/guides/architecture/enterprise#region--enterprise . But a simple explanation is that region is a numerical prefix for all the ID values in the system.

When creating a fresh database instead of importing an existing one, if a REGION file exists in the manageiq/ dir, and contains a number, the database will be created with that region setting, otherwise it will default to 0.

In region 0, from_cid and to_cid do nothing, and all objects will have purely numerical ids (like, 14).

In regions other than 0, any id will be usually shortened in the form of 16r14 for an equivalent id in region 16. (The expanded form being 16000000000014.) And from_cid is needed to convert that 16r14 back to 16000000000014, while to_cid does the opposite transformation.

this is a re-factor!

Understood, but why not make things better than refactoring? :) Or, in this case, at least useable :).

And as long as I can fill out all the fields, sucessfuly click submit, and only then get an error, the form is not quite user friendly. And when that error is a generic exception with Invalid input, not a flash message, and not mentioning a specific field that's wrong.. I think we can both agree that this is something that needs to be fixed, so why not now.

If you want to split this into a purely-controllerAs-refactor PR, and a fix-the-bugs PR, feel free to :).

gildub · 2017-07-25T06:12:53Z

@himdel,

Sorry I missed the support feature which was present for the delete case but for the create/update cases, I just added them.
Thanks for creating ManageIQ/manageiq#15569 .
No worries regarding the REGION, I've added the missing from_cid.

himdel · 2017-08-02T12:08:30Z

Test failure looks familiar, restarting travis :)

himdel · 2017-08-02T13:14:34Z

app/assets/javascripts/controllers/cloud_subnet/cloud_subnet_form_controller.js

+        vm.cloudSubnetModel.network_protocol = data.network_protocol;
+        vm.afterGet = true;
+        vm.modelCopy = angular.copy( vm.cloudSubnetModel );
+      }).catch(miqService.handleFailure);
      miqService.sparkleOff();


This does not belong here, should be inside that .then function body.. Otherwise, there is no spinner.

himdel · 2017-08-02T13:57:27Z

himdel · 2017-08-02T13:59:54Z

Hey, added the checklist here as well, looks like you just removed the manageiq.angular.scope line, so please return it back, pointing to vm.

As for merging new&edit .. if this doesn't make sense to do in this PR, let me know, I can merge as is (except the scope thing).

The same goes for object.assign - would be nice to solve this here, but if there is a reason not to...

EDIT: but since you're doing changes anyway, I also notice the Placement h3 is off (too much to the left, compared with the others), looks like it should be outside that form-group, not inside.

Replaces $scope with this Adds ng-cloak and form-change Removes obsolete miqrequire and so

gildub · 2017-08-09T01:45:28Z

@himdel,

I added back the missing ManageIQ.angular.scope, fix the Spinner.
The H3 alignment is an issue because it needs to appear only after the Network Manager has been selected, in my latest test it looked good, but that's a really annoying one.

Yes I would rather focus on the refactor only part here. Rest assured I'm going to work on merging edit/new and use Object.assign.

Thanks

miq-bot · 2017-08-09T01:48:52Z

Checked commits https://github.com/gildub/manageiq-ui-classic/compare/51d030caef282c2c49e5717f6474551db7803a62~...cd0b6e6f4ae0bc08dfd563504c5f75434591026d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

himdel · 2017-08-11T11:56:13Z

Thanks :) Merged.. but please do look into adding some validations here.

gildub · 2017-08-14T03:57:20Z

@himdel, thanks, will do.

miq-bot · 2017-10-06T02:27:24Z

@gildub Cannot apply the following label because they are not recognized: fine/yes bug

gildub · 2017-10-08T23:57:10Z

The following PRs are taking care of the backport for Fine branch:
ManageIQ/manageiq#16150
#2320

Removing fine/yes and 'bug' labels since the later patches are addressing the issue.

miq-bot · 2017-10-08T23:59:04Z

@gildub unrecognized command 'remove', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

simaishi · 2017-11-20T19:05:25Z

Backported to Fine via #2320

gildub changed the title ~~Cloud subnets refactor~~ [WIP]Cloud subnets refactor Jun 15, 2017

miq-bot added the wip label Jun 15, 2017

chessbyte added compute/cloud refactoring labels Jun 15, 2017

gildub changed the title ~~[WIP]Cloud subnets refactor~~ Cloud subnets refactor Jun 16, 2017

miq-bot removed the wip label Jun 16, 2017

dclarizio assigned himdel Jun 19, 2017

gildub mentioned this pull request Jun 20, 2017

Network provider: Cloud subnets use API #1508

Merged

gildub changed the title ~~Cloud subnets refactor~~ Network Provider: Cloud subnets refactor Jun 20, 2017

miq-bot added the networks label Jun 20, 2017

himdel reviewed Jun 28, 2017

View reviewed changes

miq-bot added unmergeable and removed unmergeable labels Jul 5, 2017

gildub closed this Jul 14, 2017

himdel closed this Aug 2, 2017

himdel reopened this Aug 2, 2017

himdel reviewed Aug 2, 2017

View reviewed changes

gildub added 3 commits August 9, 2017 11:28

Network Provider: Cloud subnets refactor

51d030c

Replaces $scope with this Adds ng-cloak and form-change Removes obsolete miqrequire and so

Add missed feature support and from_cid

865c4d1

ManageIQ.angular.scope back; Spinner fix

cd0b6e6

himdel merged commit 69bec0a into ManageIQ:master Aug 11, 2017

himdel added this to the Sprint 67 Ending Aug 21, 2017 milestone Aug 11, 2017

gildub deleted the Cloud_Subnets-refactor branch August 17, 2017 02:04

gildub restored the Cloud_Subnets-refactor branch October 6, 2017 02:23

miq-bot added bug fine/yes labels Oct 6, 2017

gildub deleted the Cloud_Subnets-refactor branch October 6, 2017 02:40

miq-bot removed fine/yes bug labels Oct 9, 2017

gildub mentioned this pull request Nov 3, 2017

[FINE] Cloud Subnet issue with save button #2320

Merged

simaishi added the fine/backported label Nov 20, 2017

Network Provider: Cloud subnets refactor #1545

Network Provider: Cloud subnets refactor #1545

Conversation

gildub commented Jun 15, 2017 • edited

gildub commented Jun 17, 2017

miq-bot commented Jun 20, 2017

miq-bot commented Jun 20, 2017

gildub commented Jun 20, 2017

himdel commented Jun 23, 2017

gildub commented Jun 28, 2017

himdel Jun 28, 2017 • edited

Choose a reason for hiding this comment

gildub Jun 30, 2017

Choose a reason for hiding this comment

himdel Jul 11, 2017

Choose a reason for hiding this comment

himdel commented Jun 28, 2017

himdel commented Jun 28, 2017 • edited

himdel Jun 28, 2017

Choose a reason for hiding this comment

gildub Jul 25, 2017

Choose a reason for hiding this comment

gildub commented Jul 5, 2017

miq-bot commented Jul 5, 2017

gildub commented Jul 6, 2017

himdel commented Jul 11, 2017

gildub commented Jul 12, 2017

himdel commented Jul 12, 2017 • edited

gildub commented Jul 14, 2017 • edited

himdel commented Jul 14, 2017

gildub commented Jul 25, 2017

himdel commented Aug 2, 2017

himdel Aug 2, 2017

Choose a reason for hiding this comment

gildub Aug 9, 2017

Choose a reason for hiding this comment

himdel commented Aug 2, 2017 • edited

himdel commented Aug 2, 2017 • edited

gildub commented Aug 9, 2017 • edited

miq-bot commented Aug 9, 2017

himdel commented Aug 11, 2017

gildub commented Aug 14, 2017

miq-bot commented Oct 6, 2017

gildub commented Oct 8, 2017

miq-bot commented Oct 8, 2017

simaishi commented Nov 20, 2017

gildub commented Jun 15, 2017 •

edited

himdel Jun 28, 2017 •

edited

himdel commented Jun 28, 2017 •

edited

himdel commented Jul 12, 2017 •

edited

gildub commented Jul 14, 2017 •

edited

himdel commented Aug 2, 2017 •

edited

himdel commented Aug 2, 2017 •

edited

gildub commented Aug 9, 2017 •

edited